layer: clarify attributes for implied directories #970
Conversation
neersighted
requested review from
jonjohnsonjr,
jonboulle,
stevvooe,
sudo-bmitch,
sajayantony,
vbatts and
cyphar
as code owners
|
Does this mean |
|
|
|
Hmm, I suppose I misspoke, as |
|
When I first saw this, I was thinking of the Since we are saying layer creators SHOULD include the parent folders, and this is just an exception case, then this LGTM. I've added it to tomorrow's meeting agenda to get some more eyes on it. |
|
Capturing from the call -
|
|
I'm still mulling over what changes to this PR should look like, but as I experiment and consider, others might benefit from links to existing implementations:
|
|
To rephrase the shared layer concern: layers aren't attached to a single image, they can be included in multiple images. So when runtimes share layers, the filesystem permissions and ownership can vary based on the order the images are pulled. The fix is to pick settings not based on the image, either static values (root), or imply values based on the first entry in the tar file that creates the implied directories. |
|
umoci uses 0755 and the root UIDs, btw. We also set the ctime and mtime to the Unix epoch (to have at least some hope of producing a reproducible container) but I think no-one else does that. I don't think we should say that implementations shouldn't modify |
The image specification currently does not describe how conformant implementations should handle the case of a layer that contains "implied directories" -- entries that imply parent directories exist through their path, without those parent directories having their own entires in the archive. As such, this behavior is currently implementation-defined and may not be consistent, even in the same implementation (e.g. moby/moby#44106). To resolve this, we explicitly define what behavior is expected in this situation, selecting 'neutral' attributes (e.g. using the container `USER`'s UID/GID, and using `0755` for mode, as derived from the default `umask(2)` of 0022). Signed-off-by: Bjorn Neergaard <[email protected]>
|
I've spent a bit of time experimenting over the last few days, and come to some conclusions:
I've pushed up a new version that adjusts things to |
|
|
||
| As the tar format describes directory hierarchies using a flat datastructure, it is possible to have so-called "implied directories" where not all parent directories implied by an entries' path in the archive have their own entry. | ||
|
|
||
| When applying a layer, implementations MUST create any parent directories implied by an entries' path, even if it is otherwise absent from the archive. Attributes of the created parent directories MUST be set as follows: |
There was a problem hiding this comment.
I think this first MUST is uncontroversial, but the second MUST is potentially sticky and perhaps needs to be SHOULD?
To state that another way, are we confident that all existing implementations are currently complying with this second MUST? (I'm reasonably confident they are complying with the first one, because it's kind of unavoidable.)
There was a problem hiding this comment.
I agree that the attribute list needs to be a SHOULD, if only because requiring empty xattrs can run into issues (for instance, SELinux labels and NFSv4 xattrs both have weird behaviours in this respect).
|
I'd like to hear more about the motivation behind these specific recommendations/requirements. A user might naively expect parent directories to be created with the same ownership and permissions as the file being created, and I can't immediately think of an obvious reason to not do that. A different expectation might be that the ownership percolates down from the parent of the first implied directory. i.e. If you're creating |
| * `uid` is set to the `0` | ||
| * `gid` is set to the `0` | ||
| * `mode` is set to `0755` | ||
| * `xattrs` are empty |
There was a problem hiding this comment.
This cannot be MUST because some xattrs are auto-set by the kernel or need to be set in order for the system to work properly (SELinux labels for instance). I agree with @tianon that this should be a SHOULD.
(I wrote this draft message last year and it got lost in my tabs. I only just got around to posting it. Sorry...) |
|
@sparr At the moment the behaviour is completely unspecified so different image-spec implementations can produce different results. If you feel that your suggested algorithm makes more sense, then you should still want some text in the spec to describe the required behaviour. As for your suggestion, there are a few issues that would need to be considered:
My personal view is that there needs to just be a simple fallback behaviour for this edge-case -- the suggestions of copying attributes up or down just adds more complexity for something that basically no image is going to run into. No image generation tool that I'm aware of generates tar archives with implied directories (arguably it would be perfectly correct for an image-spec tool today to error out if you had an archive with implied directories). In addition, a new MUST change like this should take into account what existing tools do -- AFAIK most tools either handle this in the "naive" way (without noticing this might be an issue, they just make implied directories) or they use similar defaults to the ones described in this PR. |
|
Friendly ping @neersighted -- I think changing that one MUST to SHOULD is the only feedback preventing this from moving 👀 ❤️ |
sudo-bmitch
dismissed
their stale review
Dismissing review pending xattrs resolution.
The image specification currently does not describe how conformant implementations should handle the case of a layer that contains "implied directories" -- entries that imply parent directories exist through their path, without those parent directories having their own entires in the archive.
As such, this behavior is currently implementation-defined and may not be consistent, even in the same implementation (e.g. moby/moby#44106).
To resolve this, we explicitly define what behavior is expected in this situation, selecting 'neutral' attributes (e.g. using the container
USER's UID/GID, and using0755for mode, as derived from the defaultumask(2)of 0022).It has been observed that it may be desirable to allow (
MAY) conformant implementations to instead elect to not handle this case, and to fail with a useful error message instead. I lack the insight into the wider ecosystem to consider if this is a useful behavior, but it provides another aspect to consider when discussing this change.